🔎 Correctness — v6SubnetWithSuffix builds an invalid IPv6 prefix string and will always fail parsing. You concatenate "::" with a suffix that itself includes a leading colon, producing strings like "::::ff00::/122". This causes netip.ParsePrefix to error, and the IPv6 e2e tests will fail early.

Fix: strip the leading colon from the suffix before concatenation, or accept suffix strings without a leading colon. For example:

func v6SubnetWithSuffix(parent *net.IPNet, suffixCIDR string) (string, error) {
    parentAddr, ok := netip.AddrFromSlice(parent.IP.To16())
    if !ok {
        return "", fmt.Errorf("invalid parent IPv6 address %s", parent.IP)
    }
    suf := strings.TrimPrefix(suffixCIDR, ":")
    suffix, err := netip.ParsePrefix("::" + suf)
    if err != nil {
        return "", fmt.Errorf("invalid suffix %q: %w", suffixCIDR, err)
    }
    pBytes := parentAddr.As16()
    sBytes := suffix.Addr().As16()
    for i := range pBytes {
        pBytes[i] |= sBytes[i]
    }
    return fmt.Sprintf("%s/%d", netip.AddrFrom16(pBytes), suffix.Bits()), nil
}

Alternatively, redefine the constants without the leading colon and keep the existing code.

This test is moved to kind k8st infra and moved from here.

🔎 Correctness — This assignment does not compile: new() expects a type, but HostSubnetNeighResponsesPodsAndLoadBalancers is a constant value, not a type.

fc.Spec.HostSubnetNeighResponses = new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) // invalid

Fix by taking the address of the constant (so it escapes to the heap), or use ptr.To:

v := api.HostSubnetNeighResponsesPodsAndLoadBalancers
fc.Spec.HostSubnetNeighResponses = &v
// or
fc.Spec.HostSubnetNeighResponses = ptr.To(api.HostSubnetNeighResponsesPodsAndLoadBalancers)

this is correct in golang 1.26. Copilot keeps commenting about it though.

🔎 Correctness — Proxy-neighbor managers are enabled based on a case-sensitive string comparison:

if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" {

Elsewhere (e.g., FloatingIPsEnabled) you use case-insensitive checks to tolerate different encodings. If the config layer doesn’t guarantee canonical casing, this can silently disable the feature. Consider:

if strings.EqualFold(config.HostSubnetNeighResponses, "PodsAndLoadBalancers") {
    ...
}

valid comment.

🧪 Maintainability & Tests — Use a constant or a case-insensitive comparison for the mode check instead of a magic string. Today this is:
if config.HostSubnetNeighResponses == "PodsAndLoadBalancers" { ... }
Prefer normalizing once and/or using strings.EqualFold:
if strings.EqualFold(config.HostSubnetNeighResponses, "PodsAndLoadBalancers") { ... }
Also consider defining mode constants alongside the config parsing to avoid repeating string literals.

🧪 Maintainability & Tests — You’ve extended kubernetesServiceToProto to include Status.LoadBalancer.Ingress IPs, which is great. Recommend adding a small unit test for this function to assert:

• Ingress IPs are included when present (both single and multiple).
• Type filtering still works as expected.
  This avoids relying only on FV for this conversion behavior.

🧪 Maintainability & Tests — detectKindNetworkSubnets re-implements Docker network inspection. There’s a near-duplicate in felix/fv/proxy_neigh_test.go (inspectDockerNetworkV4). Suggest factoring these helpers into a shared test utility package to reduce duplication and future drift.

🧪 Maintainability & Tests — inspectDockerNetworkV4 re-implements Docker network inspection already present in e2e tests (detectKindNetworkSubnets). To keep maintenance low, please move these helpers to a shared test util and reuse them from both FV and E2E.

🧪 Maintainability & Tests — subnetOffset duplicates the logic later re-written as subnetOffsetV4 in felix/fv/proxy_neigh_test.go. Consider moving a single implementation (and an IPv6 variant) to a shared test utility to prevent drift and make future bug fixes atomic.

🧪 Maintainability & Tests — subnetOffsetV4 duplicates the IPv4 subnet-offset logic already implemented in E2E. Please unify this helper in a shared test utility to reduce duplication and improve maintainability.

🧪 Maintainability & Tests — When starting listeners, CompleteDeferredWork re-marks the manager as dirty and returns the error for retry. Consider adding a unit test that simulates arpFactory/ndpFactory failure to verify:

• dirty is set back to true,
• the error is surfaced on first failure,
• a subsequent CompleteDeferredWork retries and succeeds.
  This makes the backoff/retry behavior regression-proof.

🧪 Maintainability & Tests — The NDP path handles DAD (unspecified source) by sending an unsolicited NA to ff02::1 and clearing Solicited. Add a unit test to cover this path to ensure future refactors don’t inadvertently change this subtle but important behavior.

🧪 Maintainability & Tests — The “LB IP on selected node” test hard-codes that 10.0.0.100 maps to node-c. This couples the test to a specific rendezvous-hash outcome and node name, making it fragile if hashing details change. Instead, compute the selected node within the test (e.g., instantiate managers for each node and ask selectNodeForIP) and assert only the selected one advertises.

🧪 Maintainability & Tests — Similarly, the “LB IP on non-selected node” test assumes that node-a is not selected for 10.0.0.100. Derive the non-selected node dynamically to avoid brittleness if hashing inputs or implementation change.

🧪 Maintainability & Tests — Given this is the only place managers are registered based on HostSubnetNeighResponses, add a small unit/integration test for the configuration gating (e.g., ensure no proxyNeighManager is registered when Disabled). That will guard against accidental regressions in start-up wiring.

🧪 Maintainability & Tests — Setting the FelixConfiguration uses new(api.HostSubnetNeighResponsesPodsAndLoadBalancers). Prefer taking the address of a typed variable or using a helper like ptr.To:
mode := api.HostSubnetNeighResponsesPodsAndLoadBalancers
fc.Spec.HostSubnetNeighResponses = &mode
This avoids compilation issues and matches the pattern used in the E2E helper.

🧪 Maintainability & Tests — dockerRun invokes exec.Command without a context or deadline. For long-running or hung docker invocations, this can stall the test runner. Consider using exec.CommandContext with a bounded timeout, or plumb a context parameter, to make failures more diagnosable and tests less flaky.

🛡️ Security — Accepts and ARPs/NDPs for user-specified Service.spec.loadBalancerIP without controller approval

• Risk: The manager treats both Status.loadBalancer.ingress IPs and Spec.loadBalancerIP (LoadbalancerIp) as eligible VIPs. While Status ingress IPs are assigned by a privileged controller, Spec.loadBalancerIP can be set by unprivileged namespace users. With this change, a user can pick an arbitrary IP inside any no-encap pool and cause some node to answer ARP/NDP for it, potentially hijacking an address (even one not allocated for LoadBalancer use) on the local L2. There is no check of IPPool AllowedUses here; the code only checks “no encap,” which may include workload-only pools.
• Mitigations:
  • Only respond for VIPs present in Status.LoadBalancer.Ingress (controller-approved); ignore Spec.loadBalancerIP entirely in proxy_neigh.
  • Alternatively, enforce that any candidate VIP is in an IPPool with AllowedUse=LoadBalancer (you’ll likely need to plumb AllowedUses into the proto.IPAMPoolUpdate and track it here).
  • Document and gate an option to allow Spec.loadBalancerIP only when a cluster-admin explicitly enables it.

🛡️ Security — Default-on setting increases exposure by replying on L2 by default

• Risk: HostSubnetNeighResponses defaults to PodsAndLoadBalancers, turning on ARP/NDP replies across host interfaces out of the box. On clusters where host uplinks are L2-adjacent to broader networks, this can unintentionally expose pod/LB IPs outside the cluster perimeter and may conflict with expected routing- or BGP-only exposure.
• Mitigations:
  • Change the default to Disabled, with explicit opt-in.
  • If default must remain enabled, pair it with strict interface scoping (allowlist) and clear docs about the exposure impact.

🛡️ Security — Potential to reply on unintended host interfaces (missing explicit exclude/allow checks here)

• Risk: The manager ignores workload veths via wlIfacesRegexp but otherwise accepts ifaceAddrsUpdate for any interface name and then opens raw sockets to answer ARP/NDP. If ifaceAddrsUpdate includes management/uplink/bridge interfaces not meant for external exposure (and InterfaceExclude is not applied upstream), this may leak pod/LB reachability onto unintended L2 segments.
• Mitigations:
  • Ensure ifaceAddrsUpdate is sourced from an ifacemonitor that already applies Felix’s InterfaceExclude; if not, apply the same exclude in this manager.
  • Consider adding a dedicated config knob to allowlist/denylist which host interfaces this feature can bind to.

🛡️ Security — No validation of IPPool AllowedUses when deciding which IPs are eligible

• Risk: isNoEncapPool() and isInNoEncapPool() only check encapsulation modes. A LoadBalancer IP that falls inside a no-encap pool but whose AllowedUses do not include LoadBalancer (e.g., a workload-only pool) would still be accepted, enabling ARP/NDP replies for an IP that was not allocated/approved for LB use.
• Mitigations:
  • Track and honor IPPool AllowedUses (augment proto.IPAMPoolUpdate to include them) and only accept:
    • Pod IPs from pools allowing Workload (current behavior is implicitly safe as they come from local WEP updates).
    • LB VIPs from pools allowing LoadBalancer.
  • As a strict alternative for LB VIPs, rely solely on Status.loadBalancer.ingress and ignore Spec.loadBalancerIP (see prior finding).

Nell — Case-sensitive compare here means a valid lower/upper-cased value won’t enable the feature (config parsing elsewhere is case-insensitive). Use EqualFold (or normalise earlier), no?

if strings.EqualFold(config.HostSubnetNeighResponses, "PodsAndLoadBalancers") {
    // ...
}

(You’ll need to import strings.)

Nell — We panic if NewRealNetlink() fails. Return error here? If we plumb an error up from newProxyNeighManager/newProxyNeighManagerWithShims and surface it via StartDataplaneDriver (or even by returning it from CompleteDeferredWork as you already do for listener start failures), we avoid taking Felix down with a panic.

Nell — This won’t compile: new(api.HostSubnetNeighResponsesPodsAndLoadBalancers) is using a constant as a type. Should we assign the const to a var and take its address?

Nell — nit: Carrying this as a raw string into the dataplane makes the later compare brittle. Would we prefer a parsed enum/bool here (or even normalise in driver.go like we do for FloatingIPs) to make the intent explicit? Not a blocker.

Nell — runARPListener/read error: we treat any error as a timeout and continue. Worth logging non-timeout errors at debug once in a while so we don’t mask persistent read errors? (not a blocking concern)

Nell — nit: OK to leave this as best-effort cleanup, but just checking intent — we log-and-skip delete failures here. Do we want to warn (or surface the error) instead of Info so a leaked pool is more visible?

Casey — I think we don’t want to panic in felix’s dataplane init path — that can take down the whole node. Best to log and disable the feature (or bubble an error up so the driver can continue without proxy-neigh), similar to other managers.

For example:

nl, err := netlinkshim.NewRealNetlink()
if err != nil {
    log.WithError(err).Error("Proxy neighbor manager disabled: failed to create netlink handle")
    return nil // or a no-op manager, or gate creation at the caller
}

If we keep the constructor signature as-is, another option is to have NewIntDataplaneDriver guard manager construction and continue without it on error.

Casey — I’m also rather surprised this extra slice is needed — we already register the managers via RegisterManager and track them in allManagers. The proxyNeighManagers []*proxyNeighManager field doesn’t seem to be used anywhere else; can this be removed to avoid redundant state?

If we do keep it, can we clarify its purpose (and where it’s read), or switch it to the existing manager lists?

Casey — Related to the above, we append the new managers to dp.proxyNeighManagers here but (AFAICT) never read that slice. I think we can drop these appends (and the field) and just rely on RegisterManager(...) like the other managers. WDYT?

Casey — I think relying on a manual --ginkgo.skip=KindNetwork is fragile — we’ve been trying to avoid self-skip and to fail explicitly with a clear reason. Could we make this behave like our other “Requires” patterns and fail-fast when the requirement isn’t met, e.g.,

Fail("This test requires local access to the docker daemon and a KIND network; run on a kind cluster or skip via labels")

Or, wire it up as a proper “RequiresKindNetwork” capability in the framework so CI selection/skips are explicit and enforced? The key thing IMO is: if the test runs in the wrong place, it should fail loudly with a helpful message.

Casey — I am a little bit skeptical of the test spinning up a docker container on the host network — it sort of breaks our “infra vs test code” boundary. I get why it’s needed here (we need an L2-adjacent peer to exercise ARP/NDP), but can we:

• Centralize this in shared e2e infra (a helper under e2e/pkg/utils or a test-infra wrapper) so the test body isn’t managing docker directly?
• Fail fast if docker isn’t accessible (and tie that to the label), instead of relying on invocation-time skips?

If we keep this here, I think it carries its weight for this feature — just want to make sure it’s discoverable and consistently guarded. WDYT?

Casey — We’re shelling out to docker network inspect from the test. Similar concern as above — can we at least add an early preflight that verifies docker is reachable and produces a very explicit failure message if not? That’ll make misconfigurations obvious.

Casey — nit: This subnetOffset helper duplicates the (very similar) subnetOffsetV4 in the FV test (felix/fv/proxy_neigh_test.go). Could we DRY this up into a single helper within the suite? Even just keeping one variant per suite (e2e vs fv) would help reduce drift.

Casey — Same duplication nit — this is the FV copy of the subnet-offset helper. If we keep both suites, can we pick one implementation per suite to avoid subtle differences over time?

Casey — nit: These “Started/Stopped proxy neighbor listener” logs are at Info. On hosts with multiple interfaces, this could get a bit noisy. I think Debug would be more appropriate for per-interface churn.

Casey — nit: Same point for the stop log — probably Debug is better.

Casey — nit: This is a Warn on failed GARP send; might be transient/noisy. Maybe Debug (or Info with rate limiting) to avoid spamming logs in failure loops?

Casey — nit: Copyright year looks off.

Should be the current year.